-
-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/20 fft #2686
Feature/20 fft #2686
Conversation
I'm not sure why this is failing. It's failing in continuous-integration/jenkins/pr-merge during unit testing on this test:
The failures look like this
which makes me suspect it's the autodiff returning NaN and the finite diffs giving a finite result very near zero. It's passing on Mac OS X locally from a clean build and also seems to be passing on Windows in the CI. Here's a link to the full error dump so you can see the start of the error. @SteveBronder or @rok-cesnovar : any idea what might be going wrong here? I was seeing errors like this when I was developing the code, but they went away when I refactored the ad tests in mix away from using the full Jacobian framework and pulled out components myself to test. Is there a way the tests might not have caught up with the latest? I'm going to resubmit just to try. |
It's failing locally for me as well (on ubuntu 20.04) running
The |
I'm a knucklehead. The reason it was working for me is this line in my
Is there a way to turn higher-order testing off without an environment variable. I really don't know how to debug what's happening now because we're just autodiffing through Eigen's implementation. I suppose the next step is to write our own derivatives, which should be too hard, since the FFT is conceptually just a matrix multiply. There's a hint here: and I can verify with code and/or local FFT experts. |
I think you can just call |
Yeah :-/ I think the only way to really do this would be to put print statements in Eigen's FFT code and call their fft stuff with fvar types. Do you think very small division could be happening somewhere? |
I'm going to try solving this by adding analytic gradients. I should've done that from the get-go, it's just my complex and vector derivatives aren't what they should be. @mbrubake's point is the key one, namely that d fft(x) = fft(dx). Just need to translate that into an adjoint update for x in y = fft(x). And presumably inv_fft works the same way as it's just a sign difference. |
@WardBrian found the definitions we need for analytic gradients: FluxML/Zygote.jl#204 (comment) The argument Not sure if we're going to need the scaling as every one of these packages seems to scale slightly differently. |
Tests are now passing, any volunteers for reviewing this? @SteveBronder maybe? |
It should be very simple. I'm just calling the FFT functions directly and autodiffing through them now. My plan is to next get analytic derivatives working. The biggest call out is that I modified the testing framework so that if you provide +infinity tolerances, tests are skipped. I then built a custom default tolerances object that turns off all of the tests other than reverse mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments and a few things I'd like clarified. How attached are you to turning off reverse mode through setting the tolerances to infinity? Would it be more difficult than I imagine to have a expect_ad_rev_only()
set of functions instead?
Not at all, it's just very flexible and lets us test whatever subset of derivatives we want. The other thing to do would be to create an object with boolean flags mirroring the tolerances and plumb that through all the calls with the tolerance object. Or maybe we just add those fields to the tolerance object? |
@SteveBronder: I resolved all 14 requested changes (see resolution notes above). So this should be ready to review again if it passes. P.S. I would like to get this merged in a working form before trying to plumb in the analytic gradients, which is going to require upgrading all the template traits. But if you'd rather try to get the analytic gradients working in the first pass, I can tell you what they are in adjoint-Jacobian form, but I don't know how to implement without the upgraded arena, etc. |
@SteveBronder I think this is ready for another review. I think I addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
Not sure why this kicked off testing again as I didn't change anything. @SteveBronder --- this is ready to check that I made all the requested changes. Thanks! |
@bob-carpenter, this PR was approved - @SteveBronder has just let you merge it in yourself. |
Summary
This PR adds functions
fft
for 1D FFTS,inv_fft
for 1D inverse FFTs,fft2
for 2D FFTs, andinv_fft2
for 2D inverse FFTs.Implementation delegates to Eigen with appropriate templating and just autodiffs through the vector operations.
Tests
For primitive tests, (a) tested values against known values from MATLAB/SciPy/R (they all agree), and (b) tested FFT(2) and inv_FFT(2) compose in both directions to produce an identity mapping.
For autodiff tests, use the test framework to test at all orders. Functionals in there deal with extracting a univariate result.
There is no error handling to test because we just pass results to Eigen, which can handle arbitrary vectors and matrices.
Side Effects
No.
Release notes
Checklist
Math issue #(issue number) Add FFT to the math library. Use Eigen's implementation. #20
Copyright holder: Simons Foundation
By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
) THIS DOESN'T WORK (see below)make doxygen
) COULD NOT GET DOXYGEN INSTALLED ON MY MAC---IT'S NOT "TRUSTED"make cpplint
) THIS ALSO FAILS:the code is written in idiomatic C++ and changes are documented in the doxygen
(n/a) the new changes are tested (no "changes", per se)
TESTING FAILURES